[client] Adds signWithEckoWallet and quicksignWithEckoWallet functions #750

ash-vd · 2023-08-07T11:26:24Z

Adds two functions to sign a transaction with the Ecko Wallet API.

Usage, where unsignedTransaction is a IPactCommand:

const signWithEckoWallet = createEckoWalletSign();
await signWithEckoWallet(unsignedTransaction);

const quicksignWithEckoWallet = createEckoWalletQuicksign();
await quicksignWithEckoWallet(unsignedTransaction);

On both signWithEckoWallet and quicksignWithEckoWallet there are several helpers available:

const isInstalled = signWithEckoWallet.isInstalled();
const isConnected = await signWithEckoWallet.isConnected();
const accountInfo = await getAccountInfo();
await signWithEckoWallet.connect();

connect() is also automatically called when signing for a transaction

vercel · 2023-08-07T11:26:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

5 Ignored Deployments

Name	Status	Preview	Updated (UTC)
alpha-docs	⬜️ Ignored (Inspect)	Visit Preview	Aug 29, 2023 9:19am
docs-storybook	⬜️ Ignored (Inspect)	Visit Preview	Aug 29, 2023 9:19am
immutable-records	⬜️ Ignored (Inspect)	Visit Preview	Aug 29, 2023 9:19am
react-ui	⬜️ Ignored (Inspect)	Visit Preview	Aug 29, 2023 9:19am
tools	⬜️ Ignored (Inspect)	Visit Preview	Aug 29, 2023 9:19am

Takadenoshi · 2023-08-16T07:46:05Z

packages/libs/client/src/signing/eckoWallet/eckoCommon.ts

+  if (checkStatusResponse?.status === 'fail') {
+    return false;
+  }
+
+  return true;


Is this the best way to check for active status or should we return true on a positive signal?

Although Ecko returns a fail status when it isn't connected, I agree that it might be better to turn things around to only return true when the returned status is success. Then it'll also return false when Ecko returns capybara or something else that's just weird.

Takadenoshi · 2023-08-16T07:46:28Z

packages/libs/client/src/signing/eckoWallet/eckoCommon.ts

+    await window.kadena?.request<IEckoConnectOrStatusResponse>({
+      method: 'kda_connect',
+      networkId,
+    });


Does this ever throw?

Only when request doesn't exist on window.kadena, which is checked in isConnected. I've now added an extra check in that function to check if window.kadena.request is there.

packages/libs/client/src/signing/eckoWallet/quicksignWithEckoWallet.ts

Takadenoshi

LGTM aside from two small questions

javadkh2 · 2023-08-21T07:55:57Z

packages/libs/client/src/signing/eckoWallet/eckoTypes.ts

+ */
+export interface IEckoSignSingleFunction
+  extends ISingleSignFunction,
+    ICommonEckoFunctions {}


mixing a function with properties might lead to a confusing api for users since function itself has some methods like apply, call ,...
what if we instead add the sign as a method like this

export interface IEckoSignFunction { isInstalled: typeof isInstalled; isConnected: typeof isConnected; connect: typeof connect; sign : ISingleFunction, quickSign : ISingleFunction, }

We've thought about that, but we wanted to make the usage of this function the same as the other functions to allow drop-in replacement.

javadkh2 · 2023-08-21T08:06:45Z

Maybe instead of exposing createEckoWalletSign and createEckoWalletQuicksign we can have one creator like createEchoWalletConnector

export interface IEckoWalletConnector {
   isInstalled: () => boolean;
   isConnected: (networkId:string) => Promise<boolean>;
   connect: (networkId:string) => Promise<boolean>;
   sign : ISingleSignFunction,
   quickSign : ISignFunction,
}
const echoWalletConnector: IEckoWalletConnector = createEchoWalletConnector();

What do you say @alber70g
Maybe we even can have a separate package for connectors? like @kadena/wallet-connectors
then we can focus of creating some connectors for common wallets there.

packages/libs/client/src/signing/eckoWallet/eckoTypes.ts

alber70g · 2023-08-21T09:50:50Z

Maybe instead of exposing createEckoWalletSign and createEckoWalletQuicksign we can have one creator like createEchoWalletConnector
export interface IEckoWalletConnector {
   isInstalled: () => boolean;
   isConnected: (networkId:string) => Promise<boolean>;
   connect: (networkId:string) => Promise<boolean>;
   sign : ISingleSignFunction,
   quickSign : ISignFunction,
}
const echoWalletConnector: IEckoWalletConnector = createEchoWalletConnector();
What do you say @alber70g Maybe we even can have a separate package for connectors? like @kadena/wallet-connectors then we can focus of creating some connectors for common wallets there.

There are multiple things at hand here

as said in above comment, we wanted to make it as uniform as possible when working with the wallets and sign functions
we want to deprecate sign at some point. It's easier to deprecate that function when it has its own separate code-branch
A separate package would be nice. This would provide a clear entry point into all the external connections that can be made and provide an easy way to integrate new connections. Let's think about this, and create an issue for that

common/changes/@kadena/client/feat-signWithEckoWallet_2023-08-08-11-29.json

LindaOrtega · 2023-08-25T16:55:31Z

Do we still want to merge this pull request?

changeset-bot · 2023-08-29T07:50:06Z

🦋 Changeset detected

Latest commit: 8748d4b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@kadena/client	Minor
@kadena/tools	Patch
@kadena/client-examples	Patch
@kadena/cookbook	Patch
@kadena/pactjs-generator	Minor
@kadena/pactjs-cli	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ash-vd requested review from alber70g, javadkh2 and webpro as code owners August 7, 2023 11:26

ash-vd force-pushed the feat/signWithEckoWallet branch from 598b7c3 to e7c03ec Compare August 7, 2023 11:37

ash-vd changed the title ~~Adds signWithEckoWallet and quicksignWithEckoWallet functions~~ [client] Adds signWithEckoWallet and quicksignWithEckoWallet functions Aug 8, 2023

Takadenoshi reviewed Aug 16, 2023

View reviewed changes

EnoF reviewed Aug 16, 2023

View reviewed changes

packages/libs/client/src/signing/eckoWallet/quicksignWithEckoWallet.ts Show resolved Hide resolved

Takadenoshi approved these changes Aug 16, 2023

View reviewed changes

ash-vd requested review from eileenmguo, sstraatemans and timoheddes as code owners August 16, 2023 09:10

EnoF approved these changes Aug 16, 2023

View reviewed changes

ash-vd force-pushed the feat/signWithEckoWallet branch from 3a5f6fb to 7aeda2a Compare August 16, 2023 14:43

javadkh2 reviewed Aug 21, 2023

View reviewed changes

packages/libs/client/src/signing/eckoWallet/eckoTypes.ts Show resolved Hide resolved

alber70g force-pushed the feat/signWithEckoWallet branch from add6eb5 to 9a1f564 Compare August 21, 2023 09:57

alber70g reviewed Aug 21, 2023

View reviewed changes

common/changes/@kadena/client/feat-signWithEckoWallet_2023-08-08-11-29.json Outdated Show resolved Hide resolved

alber70g force-pushed the feat/signWithEckoWallet branch from 9a1f564 to 9e78532 Compare August 21, 2023 10:02

alber70g approved these changes Aug 21, 2023

View reviewed changes

alber70g force-pushed the feat/signWithEckoWallet branch from 9e78532 to d938330 Compare August 21, 2023 10:09

javadkh2 mentioned this pull request Aug 23, 2023

Move all wallet specific helpers to @kadena/wallet #812

Open

3 tasks

alber70g force-pushed the feat/signWithEckoWallet branch 4 times, most recently from 3803545 to 3304085 Compare August 29, 2023 07:50

ash-vd and others added 6 commits August 29, 2023 11:18

Adds signWithEckoWallet and quicksignWithEckoWallet functions

3f63cfd

Remove getAccountInfo, add more tests

befc4c0

Update to client 1.0.0

889c253

PR comments

0c8c6ae

Export types

89c02a4

chore(client): adds changeset

8748d4b

alber70g force-pushed the feat/signWithEckoWallet branch from 3304085 to 8748d4b Compare August 29, 2023 09:18

alber70g merged commit b8b8661 into main Aug 29, 2023
4 checks passed

alber70g deleted the feat/signWithEckoWallet branch August 29, 2023 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[client] Adds signWithEckoWallet and quicksignWithEckoWallet functions #750

[client] Adds signWithEckoWallet and quicksignWithEckoWallet functions #750

ash-vd commented Aug 7, 2023

vercel bot commented Aug 7, 2023 •

edited

Loading

Takadenoshi Aug 16, 2023

ash-vd Aug 16, 2023 •

edited

Loading

Takadenoshi Aug 16, 2023

ash-vd Aug 16, 2023

Takadenoshi left a comment

javadkh2 Aug 21, 2023

alber70g Aug 21, 2023

javadkh2 commented Aug 21, 2023 •

edited

Loading

alber70g commented Aug 21, 2023

LindaOrtega commented Aug 25, 2023

changeset-bot bot commented Aug 29, 2023 •

edited

Loading

[client] Adds signWithEckoWallet and quicksignWithEckoWallet functions #750

[client] Adds signWithEckoWallet and quicksignWithEckoWallet functions #750

Conversation

ash-vd commented Aug 7, 2023

vercel bot commented Aug 7, 2023 • edited Loading

Takadenoshi Aug 16, 2023

Choose a reason for hiding this comment

ash-vd Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Takadenoshi Aug 16, 2023

Choose a reason for hiding this comment

ash-vd Aug 16, 2023

Choose a reason for hiding this comment

Takadenoshi left a comment

Choose a reason for hiding this comment

javadkh2 Aug 21, 2023

Choose a reason for hiding this comment

alber70g Aug 21, 2023

Choose a reason for hiding this comment

javadkh2 commented Aug 21, 2023 • edited Loading

alber70g commented Aug 21, 2023

LindaOrtega commented Aug 25, 2023

changeset-bot bot commented Aug 29, 2023 • edited Loading

🦋 Changeset detected

vercel bot commented Aug 7, 2023 •

edited

Loading

ash-vd Aug 16, 2023 •

edited

Loading

javadkh2 commented Aug 21, 2023 •

edited

Loading

changeset-bot bot commented Aug 29, 2023 •

edited

Loading